Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for managing uploaded media #1323

Merged
merged 22 commits into from
Jan 2, 2025

Conversation

micahmo
Copy link
Member

@micahmo micahmo commented Apr 23, 2024

Pull Request Description

This PR adds the ability to delete uploaded images. There is a new section under account settings for managing media. Navigating to this new page will display all uploaded images with the option to delete any of them. This is only supported for instances running >= 0.19.4.

See: thunder-app/lemmy_api_client#21.

Issue: #1447

Screenshots / Recordings

manage-media-demo.mp4

P.S. While working on this feature, I discovered something really interesting! I've always been annoyed at how seemingly quickly our image caches reset despite having the new aggressive image caching setting. It turns out that this is less about image caching and more about widget caching. The default behavior of SliverLists is to pretty aggressive dispose and rebuild widgets as they're scrolled off/on screen, and this naturally causes some loading artifacts, even for cached images. However, this trick seems to make things much better.

Pass these paramters to the SliverList.

addSemanticIndexes: false,
addAutomaticKeepAlives: false,
addRepaintBoundaries: false,

And then wrap whatever is the root widget in the list with a KeepAlive(keepAlive: true).

For now I've only done it here, but maybe we could consider doing this in other places of the app (respecting the aggressive image caching option). Here's a before/after of just that difference.

Before

scroll-reload-before.mp4

After

scroll-reload-after.mp4

@micahmo
Copy link
Member Author

micahmo commented Apr 24, 2024

I decided to add on a slight improvement to this feature. I added the ability to search for usages of the media link. It can help you to decide whether you really want to delete it. It should return cases where the link appears as the URL of a post, in the body of a post, or in the body of a comment. Of course, this method is not foolproof, as the link could be used in some other way (including not even on Lemmy). But it should at least help the user to make a more informed decision.

qemu-system-x86_64_TahRys0qlj.mp4

@micahmo micahmo mentioned this pull request Jun 14, 2024
7 tasks
@micahmo
Copy link
Member Author

micahmo commented Jun 25, 2024

@hjiangsu Would you be able to give me a hand figuring out why the tests are failing here? They're saying something about not finding freezed. But I didn't remove that package from the pubspec here, nor in the corresponding change in the API repo. So I'm a bit stuck! 😊

@hjiangsu
Copy link
Member

Hmm, not entirely sure what's causing the failures. I glanced at the CI logs and did see this: https://github.com/thunder-app/thunder/actions/runs/9664871877/job/26661495345?pr=1323#step:5:170

I'm not sure if this is what's causing the subsequent failures, but it could be something!

image

@micahmo
Copy link
Member Author

micahmo commented Jun 25, 2024

I saw that too, which is what made me think I removed it by accident, but I didn't. 😆 Maybe I should just add it?

@micahmo
Copy link
Member Author

micahmo commented Jun 25, 2024

Maybe I should just add it?

Ok that seems to have worked! It really just changed the reference type in the pubspec.lock from transitive to direct main.

Now it's failing at the linting step, despite apparently not showing any errors (only info/warning). Lol!

EDIT: Nevermind, I see the error. Not sure why I'm not getting it locally.

https://github.com/thunder-app/thunder/actions/runs/9666355363/job/26665643096?pr=1323#step:10:198

@micahmo
Copy link
Member Author

micahmo commented Jun 26, 2024

Finally got this building! 😌

@micahmo micahmo force-pushed the feature/delete-media branch from 1d670bb to da9b900 Compare July 12, 2024 22:10
@micahmo
Copy link
Member Author

micahmo commented Oct 21, 2024

@hjiangsu Alrighty, this is my next PR based on age. I know this is a big one in terms of changes, but it's one that I'm very excited to have. I think it's especially useful since it's related to the new (well, not so new any more 😆) Lemmy version, so folks might be expecting this behavior to be available once their instance is upgraded. Let me know what you think!

@hjiangsu
Copy link
Member

hjiangsu commented Jan 2, 2025

@micahmo Sorry for the delay on this one again - would it be possible for you to fix the merge conflicts here?

After the merge conflicts are fixed, I'll go ahead and merge this one in!

Copy link
Member

@hjiangsu hjiangsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for implementing this and for fixing the merge conflicts!

@hjiangsu hjiangsu merged commit 5ebc407 into thunder-app:develop Jan 2, 2025
1 check passed
@micahmo
Copy link
Member Author

micahmo commented Jan 2, 2025

Just FYI, this isn't building for me locally and I'm not sure why (so I wasn't going to ask for it to be merged yet). But the CI passed, so I'm not too worried. Does it build for you?

@micahmo micahmo deleted the feature/delete-media branch January 2, 2025 22:27
@hjiangsu
Copy link
Member

hjiangsu commented Jan 2, 2025

Ahh, you know what - I forgot to update the CI to use 3.27, so it builds properly in 3.24 but it doesn't in 3.27. This is likely due to pubspec.lock not being updated properly.

Try running flutter pub upgrade and build again! I'll create a PR for this

@micahmo
Copy link
Member Author

micahmo commented Jan 2, 2025

Ahh yep, that was it! I was able to build and re-test that everything works after the merge. I'll wait for your PR to officially update the pubspec.lock. Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants